We already test the sdist contents, see https://github.com/python-trio/trio/blob/master/ci.sh#L131-L148 which has the advantage of also working locally. I'll close preemptively but feel free to reopen.

I had missed that you want to allow others to test using only the sdist, but including a random certificate in our sdist seems wrong even if that mean downstream won't be able to test LSP support.

(Maybe others disagree, in which case I will be happy to reopen.)

@pquentin well, the tests need to be usable if they are included in sdist. If you don't like the certificate, maybe we could exclude it and have some way to skip said tests, keeping the rest in a good shape. After all, the downstreams are usually Linux-based anyway. OTOH, if a “random certificate” is a prerequisite for tests, it's not really “random” but is a part of the testing setup.

I'll reopen this for now to get more visibility. I firmly believe that sdists should ship everything necessary to run tests and be self-contained in general. Otherwise, an implicit dependency on Git may appear, and it won't be noticeable in many envs.

I firmly believe that sdists should ship everything necessary to run tests and be self-contained in general.

In fact, this PR demonstrates that a part of the pre-requisites is indeed missing from the sdist.

While I generally like the approach of testing the artifacts instead of the source, there's a basic hazard that I think this doesn't address. Or maybe I looked too quickly. What if some tests are just missing or get skipped etc? I would expect some verification that the artifact has all the proper tests compared to the source checkout.

@altendky that's a good point. I usually use setuptools-scm which guarantees that all Git-tracked files will end up in the sdist, which is why it doesn't usually occur to me that this could be a problem.

I've given some thought to what you said. My first instinct was to add a CI check that would retrieve the number of executed tests from the JUnit output that pytest produces and ensure that it's sufficiently big. Then, I remembered seeing a pytest plugin that allows specifying a minimum number of tests required to succeed for the test session to be successful. Never used this one personally, though.

The more I thought about it, the more I realized that the solution is on the surface — the coverage metric would drop dramatically if the tests stop being executed. Specifically, the test directory should report 100% coverage. I recall earlier discussions in the community on whether to measure coverage on the tests themselves, and nedbat even has a blog post explaining that this is important.

I've gone to https://app.codecov.io/gh/python-trio/trio/tree/master/trio/tests and, to my surprise, its coverage is at 96.03% — that's no good!

Looking inside, I see that there's a few reasons that things are not covered:

1. branchy tests, violating the best practice of simplicity — the more logic is added to the test functions, the higher the chance for the tests to be buggy
2. some tests only run under certain platforms and, for some reason, they don't (or maybe the coverage isn't being reported?); for example, https://app.codecov.io/gh/python-trio/trio/blob/master/trio/tests/test_wait_for_object.py is all-red, despite that the CI jobs for Windows exist.

Digging deeper, I see

https://codecov.io/upload/v4?package=bash-1.0.6&token=<hidden>&package=bash-1.0.6&token=&branch=&commit=f2a71a0f67b34ecfc1dea60908645dd8a17eeaa9&build=3958687617&build_url=&name=&tag=&slug=&service=github-actions&flags=&pr=&job=&cmd_args=n
{'detail': ErrorDetail(string='Missing "owner" argument. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
404
==> Uploading to Codecov
 % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                Dload  Upload   Total   Spent    Left  Speed

 0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 42291  100   150  100 42141   1726   473k --:--:-- --:--:-- --:--:--  485k
   {'detail': ErrorDetail(string='Missing "owner" argument. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
+ true

This is from https://github.com/python-trio/trio/actions/runs/3958687617/jobs/6780554545#step:4:1660.

There are some problems with how this is set up:

1. the official codecov action is not used
2. there is no transparency wrt the coverage upload
3. ci.sh uses an uploader that's been deprecated for years (related to 1)

The solution would be simple — install Codecov GitHub App into the repo and make coverage required check. Configure coverage to demand 100% on the test directory, at least. Use the official up-to-date uploader (directly in the script, through the action in GHA).

This will make sure that the tests are included and executed, IMO. In conjunction with explicitly adding them to MANIFEST.in or using a plugin that adds everything Git-tracked into sdist (this would mean zero need for MANIFEST.in, most of the time).

Thanks for digging into this and for identifying some other points to work on.

So WDYT about improving packaging along the way?

I'm not particularly involved here at this point so I won't promise to provide reviews or such, but I am quite used to ending up multiple layers deep in fixing stuff to get PRs through. I would personally suggest doing it as separate PRs though and not putting it all in this one.

tl;dr, sounds great!

Yeah, I didn't mean to imply that it'd be included in this PR. I'm pretty much fanatic about making PRs and commits atomics.
I was just seeing feedback on the general direction of the improvements.

1. the official codecov action is not used

2. there is no transparency wrt the coverage upload

3. ci.sh uses an uploader that's been deprecated for years (related to 1)

UPD: this has recently been fixed by making use of the official codecov action.

Codecov Report

Merging #2541 (3d9e30f) into master (bec7cc6) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2541      +/-   ##
==========================================
+ Coverage   98.10%   98.14%   +0.04%     
==========================================
  Files         118      118              
  Lines       16471    16471              
  Branches     2977     2977              
==========================================
+ Hits        16159    16166       +7     
+ Misses        255      249       -6     
+ Partials       57       56       -1     

I think maybe the formatting check can/should be excluded from this transition, and have that be tested against the Git content. I don't see any reason for downstream users to run formatting checks, and saves on having to bundle a couple files - including verify_types.json from #2629 (though after I trimmed that file down it specifically isn't much of a size concern anymore), *-requirements.in, etc.

I am curious about the status of this change, it sounds like it would be useful from the points you mentioned.

@jakkdl there's school of thought preaching that sdists should be as close to Git source as possible. And I also prefer this way — it's easier to maintain, for one. In my projects, setuptools-scm just auto-includes everything tracked by Git and I don't even have a MANIFEST.in file — it's one burden less to care about.

@CoolCat467 it fell off my radar. I thought it was kinda ready but couldn't get enough people on board to get approvals. Maybe, I'll resurrect it some time...